Valkey is redis compatible, is half the cost and faster. Can we use that?

Just waiting on agreement.

3.0.9 exists, 3.0.7 is being used here. minor, ignore if you want

i think weve historically had xray off by default in prod as its quite spendy, and enabling it as and when needed.

All variables should be prod defaults with PTL envs having overrides

please sort alphabetically, it makes me sad

There is a lot of mock setup at the beginning of this. Could we extract this out into a test helper or maybe a fixture so that the test file is cleaner and more readable?

This looks a little hard to read, could do with parens around the first operation

These destructures should be merged ideally

Ideally we either disable this lint at the top or find ways to fix this lint properly.

Not sure why we're disabling this lint?

Do we need to try it without to see if it fails?

We could do with a test being added that tests that an unexpected error being thrown on one record doesn't prevent a subsequent record being processed e.g. An unexpected error thrown by an upstream dependency like loadTargetConfig which would be caught by the try/catch in processRecords - loadTargetConfig throwing "S3 unavailable" for record 1, while record 2 should still be processed normally.

Maybe also have a negative test for this that verifies an empty payload produces a deterministic (non-empty) signature, ensuring the signing function doesn't silently fail on edge-case payloads?

For cost saving I think we should switch to valkey.
You are billed in gigabyte-hours (GB-hrs) and the minimum for redis is 1GB vs 100mb in valkey.
Not sure we'll go above 100mb even in prod.
https://aws.amazon.com/elasticache/pricing/

Just waiting on agreement.

Not related to the phase 2 work but I think we can delete lines 3-6 - those secrets have never been in our repo at any point

1GB is the minimum with redis but can go down to 100mb if we make the valkey switch.
Keeping this low for dev/test environments is good for cost saving.
We should see how much each client will take in storage.

Do we want an alarm on storage - e.g. 80% used

With passthrough we should be able route mTLS and non mTLS traffic through and remove the aws_lambda_permission config in module_mock_webhook_lambda.
We'd then need to modify the lambda to output whether it was mTLS or not which the tests can then verify based on their expectation.
This would simplify things a lot and also get us the hardening we are looking for on the mock webhook.

Looks like the lambda is also missing MTLS_TEST_CA_S3_KEY

I don't think any of these are referenced?
AI seems to like adding outputs for no reason.

As per my other comment I think we need to review the production vs non-production terminology we are using. Production should be assumed and non-production should be "dev" - this aligns with the tfvar groups.

Not used in the module.

Delete now its in a shared module. This is just a duplicate of src/config-cache/src/__tests__/config-cache.test.ts

Its never very good at cleaning up after itself. Theres a bunch of dead code to remove. AI summary:

Files to delete:

Knock-on changes required:

1. package.json — remove @aws-sdk/client-ssm from dependencies (only used in ssm-applications-map.ts and its test)

2. src/__tests__/index.component.test.ts — needs cleanup:

   • Remove the jest.mock("@aws-sdk/client-ssm", ...) block (lines 18–26)
   • Remove mockSsmSend variable and its usages
   • Remove APPLICATIONS_MAP_PARAMETER from process.env setup/teardown
   • Remove CLIENT_SUBSCRIPTION_CONFIG_BUCKET, CLIENT_SUBSCRIPTION_CONFIG_PREFIX, CLIENT_SUBSCRIPTION_CACHE_TTL_SECONDS env vars — these are now the wrong names (should be CLIENT_CONFIG_BUCKET etc. per the earlier fix), and the SSM-related ones are dead
   • Remove mockSsmSend.mockResolvedValue(...) from beforeEach
   • Remove mockSsmSend.mockClear() from beforeEach

   Note: the SSM mock was used to simulate ApplicationsMapService loading the applications map — since that's gone from this lambda, it's entirely redundant. The applicationsMap const and mockSsmSend usages can all go.

Also need to remove APPLICATIONS_MAP_PARAMETER from the terraform module for the lambda.

Remove

We need a .luarc.json file at the root level like we had on the PoC branch - otherwise the IDE won't be happy with the undefined globals
The following works:

{
  "diagnostics": {
    "globals": [
      "KEYS",
      "ARGV",
      "redis"
    ]
  }
}

Lol this is crazy to read

If this is misconfigured then the behaviour will be undesirable - SQS will just keep retrying it 100 times.
However it will never happen so I don't think its worth explicitly handling it and treating the events as permanent failures (by sending to the DLQ).

I preferred the observability stuff we had in the client transform filter lambda (lambdas/client-transform-filter-lambda/src/services/observability.ts) which wrapped the logging and metric calls into one. It cuts down on the clutter a bit by reducing it into 1 call.

We may face URL encoding issues here. https.request does not perform URL escaping, so spaces may result in malformed HTTPS requests.

Instead, since https.request supports a URL object for path, we should pass through the url object we already construct before making the request.

We should have proper constant naming here for 5, 2, etc so it's understandable at a high level

I think we should make use of a proper npm package for pem file parsing, that way we're not hand rolling our own parsing code which could result in security vulnerabilities from malformed data.

I agree - there must be something off the shelf somewhere for this.

If we use secrets manager for everything we won't have this problem.

This could be simplified like so:

cache ??= new ConfigCache((Number(process.env.CLIENT_SUBSCRIPTION_CACHE_TTL_SECONDS) || 300) * 1000);

I don't think we should be re-exporting like this, we should instead directly use the @nhs-notify-client-callbacks/logger import.

Why is it we're dividing by 1000 just to then multiply by 1000 later? It may make more sense to make the default be in seconds to simplify all of this.

Why are we doing an in check here? Can we strongly type result instead to avoid this?

Its the discriminated union type ok and permanent are both false so it doesn't know if its a rate limited result or other status code result.
I've suggested an alternative type which is easier to understand and uses a "type" as a discriminator.

We should try and processRecords in parallel up to a configurable concurrency limit.

This function is too long and needs refactoring as its hard to follow. Especially with the early returns if result.ok == true.
Can it be refactored so we broadly have (with other bits between):

checkAdmission()
deliverPayload()
handleDeliveryResult()

I think a lot of this should be refactored to lambdas/https-client-lambda/src/services/delivery/retry-policy.ts we then wouldn't need to export/import as much from retry-policy and keep retry specific calculations out of the root handler.

It doesn't look like any metrics are recorded when we get a 429.
Feels like we should record this.
Are there any other important gaps in the flow where metrics aren't recorded?